Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[bazel] Use tmp dir for intermediate NVCC source files #22950

Merged
merged 3 commits into from
Mar 18, 2019
Merged

[bazel] Use tmp dir for intermediate NVCC source files #22950

merged 3 commits into from
Mar 18, 2019

Conversation

wateryzephyr
Copy link
Contributor

  • When using the local cuda cross-compilation toolchain,
    header file changes to cc_library would not trigger rebuilds for
    dependent cc_binary rules.

More info can be provided if needed to repro the bug in question.

@wateryzephyr
[bazel] Use tmp dir for intermediate NVCC source files
70fa826 
* When using the local cuda cross-compilation toolchain,
  header file changes to cc_library would not trigger rebuilds for
  dependent cc_binary rules.
@Harshini-Gadige Harshini-Gadige self-assigned this Oct 12, 2018
@Harshini-Gadige Harshini-Gadige added the awaiting review Pull request awaiting review label Oct 12, 2018
@wateryzephyr
Copy link
Contributor Author

This issue seems to be gone on e.g. commit c379cd22b88c729150d70a33a36846d2fa26b4d6. I'm curious what the fix was, as it seems different from the one in this PR?

@Harshini-Gadige
Copy link

@gunan PTAL

@gunan
Copy link
Contributor

gunan commented Dec 7, 2018

Not familiar with this bug, I will let @meteorcloudy comment.
I am very sorry about the delay, as last few months I was not able to look into github issues and PRs as much as I would like to.

meteorcloudy
meteorcloudy reviewed Dec 7, 2018
View reviewed changes
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for this change! Just a small comment.

third_party/gpus/cuda_configure.bzl
def _get_nvcc_tmp_dir_for_unix(repository_ctx):
"""Return the UNIX tmp directory for nvcc to generate intermediate source files."""
escaped_tmp_dir = escape_string(
get_env_var(repository_ctx, "TMPDIR", "/tmp"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add TMP and TMPDIR here:

tensorflow/third_party/gpus/cuda_configure.bzl

Lines 1569 to 1574 in c2e2ece

environ = [
_GCC_HOST_COMPILER_PATH,
_CLANG_CUDA_COMPILER_PATH,
"TF_NEED_CUDA",
"TF_CUDA_CLANG",
_TF_DOWNLOAD_CLANG,

So that when the environment variable changes, the CROSSTOOL will get reconfigured.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Dec 7, 2018
@Harshini-Gadige Harshini-Gadige added the awaiting review Pull request awaiting review label Dec 13, 2018
@rthadur rthadur added the size:S CL Change Size: Small label Jan 2, 2019
gunan
gunan previously approved these changes Feb 23, 2019
View reviewed changes
gunan
gunan previously approved these changes Feb 23, 2019
View reviewed changes
@gunan gunan added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process and removed awaiting review Pull request awaiting review labels Feb 23, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 23, 2019
@Harshini-Gadige Harshini-Gadige added the awaiting review Pull request awaiting review label Mar 1, 2019
@tensorflowbutler
Copy link
Member

Nagging Reviewer @gunan, @meteorcloudy: You have been added as a reviewer to this pull request. Please add your review or reassign. It has been 14 days with no activity and the awaiting review label has been applied.

@meteorcloudy meteorcloudy added the kokoro:force-run Tests on submitted change label Mar 18, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 18, 2019
@Harshini-Gadige Harshini-Gadige removed awaiting review Pull request awaiting review ready to pull PR ready for merge process labels Mar 18, 2019
@Harshini-Gadige Harshini-Gadige added the ready to pull PR ready for merge process label Mar 18, 2019
@tensorflow-copybara tensorflow-copybara merged commit 347bc00 into tensorflow:master Mar 18, 2019
tensorflow-copybara pushed a commit that referenced this pull request Mar 18, 2019
@tensorflower-gardener
Merge pull request #22950 from cgoy-nvidia:fix_hdrs_rebuilds
47f78f1 
PiperOrigin-RevId: 239009781
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@meteorcloudy meteorcloudy meteorcloudy approved these changes

@gunan gunan gunan left review comments

Assignees

@Harshini-Gadige Harshini-Gadige

Labels
cla: yes ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@wateryzephyr @Harshini-Gadige @gunan @tensorflowbutler @meteorcloudy @googlebot @kokoro-team @tensorflow-copybara @rthadur